Closed
Bug 661282
Opened 13 years ago
Closed 13 years ago
make xpcshell run on android
Categories
(Core :: XPConnect, defect)
Tracking
()
RESOLVED
FIXED
mozilla8
People
(Reporter: blassey, Assigned: blassey)
References
Details
Attachments
(3 files, 5 obsolete files)
2.42 KB,
patch
|
cmtalbert
:
review-
|
Details | Diff | Splinter Review |
2.68 KB,
patch
|
benjamin
:
review+
|
Details | Diff | Splinter Review |
743 bytes,
patch
|
benjamin
:
review+
gbrown
:
feedback+
|
Details | Diff | Splinter Review |
There are two discrete things in this patch. The first are changes to xpcshell and the XRE api to make xpcshell aware of omnijars. The second is a script to copy stuff to device and run xpcshell. I'll probably separate the two out when its time for review.
Assignee | ||
Comment 1•13 years ago
|
||
Assignee: nobody → blassey.bugs
Attachment #536652 -
Attachment is obsolete: true
Attachment #536717 -
Flags: review?(benjamin)
Assignee | ||
Comment 2•13 years ago
|
||
Attachment #536718 -
Flags: review?(ctalbert)
Assignee | ||
Comment 3•13 years ago
|
||
this patch fixes the usage of NS_ERROR with these patches, this command line: python ../build/mobile/run-xpcshell.py ~/src/mozilla-central/objdir-droid-dbg /data/app/org.mozilla.fennec_unofficial-1.apk -s -e "\"dump('hello\n')\"" produces this output (with a debug build): adbd is already running as root "cd /data/local/fennec/dist/lib/ && LD_LIBRARY_PATH=/data/local/fennec/dist/lib/ GRE_HOME=/data/data/org.mozilla.fennec_unofficial ./xpcshell --greomni /data/app/org.mozilla.fennec_unofficial-1.apk -e "dump('hello\n')"" WARNING: XPCOM objects created/destroyed from static ctor/dtor: file /home/blassey/src/mozilla-central/xpcom/base/nsTraceRefcntImpl.cpp, line 172 got jar reader: 0x40215800 WARNING: No default pref files found.: file /home/blassey/src/mozilla-central/modules/libpref/src/Preferences.cpp, line 789 hello WARNING: nsExceptionService ignoring thread destruction after shutdown: file /home/blassey/src/mozilla-central/xpcom/base/nsExceptionService.cpp, line 197 nsStringStats => mAllocCount: 1408 => mReallocCount: 7 => mFreeCount: 1408 => mShareCount: 6937 => mAdoptCount: 53 => mAdoptFreeCount: 53 WARNING: XPCOM objects created/destroyed from static ctor/dtor: file /home/blassey/src/mozilla-central/xpcom/base/nsTraceRefcntImpl.cpp, line 172
Attachment #536717 -
Attachment is obsolete: true
Attachment #536717 -
Flags: review?(benjamin)
Attachment #536761 -
Flags: review?(benjamin)
Assignee | ||
Comment 4•13 years ago
|
||
this patch adds a 3rd required argument for the package name
Attachment #536718 -
Attachment is obsolete: true
Attachment #536718 -
Flags: review?(ctalbert)
Attachment #536774 -
Flags: review?(ctalbert)
Updated•13 years ago
|
OS: Linux → Android
Hardware: x86_64 → All
Assignee | ||
Comment 5•13 years ago
|
||
the objdir argument was being ignored, fixed that
Attachment #536774 -
Attachment is obsolete: true
Attachment #536774 -
Flags: review?(ctalbert)
Attachment #536785 -
Flags: review?(ctalbert)
Comment 6•13 years ago
|
||
The script is missing a mkdir for /data/local/fennec/dist/lib.
Comment 7•13 years ago
|
||
Comment on attachment 536761 [details] [diff] [review] patch for xpcshell support for omnijar Why doesn't this end up initializing omnijar twice? I don't understand exactly why xpcshell can't find omnijar on android by default, but I really don't understand how this is safe: mozilla::Omnijar::Init is not safe for dual init, as far as I can tell. Also I really prefer XRE_InitOmnijar to take localfiles, not char*.
Attachment #536761 -
Flags: review?(benjamin) → review-
Comment on attachment 536785 [details] [diff] [review] patch to add script to run on android Review of attachment 536785 [details] [diff] [review]: ----------------------------------------------------------------- Sorry it took so long to get to the review. What you've got here is a great start to getting xpcshell running on the device. I think that there is a bit of stuff in the patch that looks like its catered to your system. Once that's cleaned up I think this will work as a general way to launch xpcshell on device. I have two general questions: * Did you try this on a tegra? I got "CANNOT LINK EXECUTABLE" when I tried it there. But I've had trouble running straight C++ code on tegras before, so it could work fine on a real phone. * A bunch of xpcshell test files print stuff to the console for debugging and status. Were you planning to extend this to handle that ability or to allow dumping the stdout/stderr of xpcshell onto a file on the phone which could be pulled back to the local dir from the calling computer? (could be a follow-on bug). ::: build/mobile/run-xpcshell.py @@ +9,5 @@ > + > +def setup(): > + # the main code goes here > + objdir=sys.argv[1] > + dirList=os.listdir(objdir + "/dist/fennec") Is this indeed the proper directory? I believe that this works for you, but I don't have a dist/fennec in my fennec objdir. I'm rebuilding now (my build was old) so perhaps that has changed. Are you doing a make package in addition to the normal build? @@ +12,5 @@ > + objdir=sys.argv[1] > + dirList=os.listdir(objdir + "/dist/fennec") > + subprocess.check_call(["adb", "shell", "mkdir", "/data/local/fennec/"]) > + subprocess.check_call(["adb", "shell", "mkdir", "/data/local/fennec/dist"]) > + subprocess.check_call(["adb", "shell", "mkdir", "/data/local/fennec/dist/lib/defaults/"]) I think you have to make /data/local/fennec/dist/lib before you attempt to make lib/defaults. @@ +18,5 @@ > + for fname in dirList: > + flen = len(fname) > + if fname[flen - 3:] == ".so": > + print fname > + copy(objdir + "/dist/fennec/", "/data/local/fennec/dist/lib/", fname ); For the local system's path, I'd use os.path.join(<path>, <path>), in case someone tries this on windows. @@ +24,5 @@ > + copy(objdir + "/dist/bin/", "/data/local/fennec/dist/lib/", "xpcshell") > + > +def main(): > + if len(sys.argv) < 2: > + print "usage: python xpcshell <path to objdir> <path to apk on device> <package name> [-g] [-s] [xpcshell arguments]" nit: I don't think the xpcshell arguments are optional. Things fail if you don't have at least 4 arguments. Also, I can't figure what you'd do running xpcshell without arguments. @@ +39,5 @@ > + subprocess.check_call(["adb", "forward", "tcp:12345", "tcp:12345"]) > + cmd = "cd /data/local/fennec/dist/lib/ && LD_LIBRARY_PATH=/data/local/fennec/dist/lib/ GRE_HOME=/data/data/" > + cmd += sys.argv[3] > + if use_gdbserver: > + cmd += " /data/local/gdbserver localhost:12345" nit: Making an assumption about where gdbserver is installed, I'd mention that up in the help (Though I'm not sure it *can* be installed anywhere else on the phone...if that's the case, ignore this).
Attachment #536785 -
Flags: review?(ctalbert) → review-
Assignee | ||
Comment 9•13 years ago
|
||
(In reply to comment #7) > Comment on attachment 536761 [details] [diff] [review] [review] > patch for xpcshell support for omnijar > > Why doesn't this end up initializing omnijar twice? This check prevents omnijar from being initialized twice: http://mxr.mozilla.org/mozilla-central/source/xpcom/build/nsXPComInit.cpp#460 > I don't understand > exactly why xpcshell can't find omnijar on android by default, Why would it? This is akin to providing a library loading path and GRE_HOME > but I really > don't understand how this is safe: mozilla::Omnijar::Init is not safe for > dual init, as far as I can tell. Its not, but it doesn't happen > > Also I really prefer XRE_InitOmnijar to take localfiles, not char*. No problem
Assignee | ||
Comment 10•13 years ago
|
||
Attachment #536761 -
Attachment is obsolete: true
Attachment #542066 -
Flags: review?(benjamin)
Comment 11•13 years ago
|
||
Comment on attachment 542066 [details] [diff] [review] patch for xpcshell support for omnijar This should really be a private API, but I guess you can stick in the XRE_ namespace for now.
Attachment #542066 -
Flags: review?(benjamin) → review+
Comment 12•13 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/10625264d566
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla8
Comment 13•13 years ago
|
||
A probably unintended feature of the patch that landed is that now both --greomni and --appomni must be specified. If only --greomni is specified, I get a segmentation violation. (The original patch set appomni==greomni if only --greomni was specified.)
Assignee | ||
Comment 14•13 years ago
|
||
Geoff points out that the behavior of this changed between the first and second patch. This brings back the original handling of not specifying --appomni
Attachment #548872 -
Flags: review?(benjamin)
Attachment #548872 -
Flags: feedback?(gbrown)
Comment 15•13 years ago
|
||
Comment on attachment 548872 [details] [diff] [review] follow up patch This fixes the problem I reported - works fine for me.
Attachment #548872 -
Flags: feedback?(gbrown) → feedback+
Updated•13 years ago
|
Attachment #548872 -
Flags: review?(benjamin) → review+
Comment 16•13 years ago
|
||
http://hg.mozilla.org/integration/mozilla-inbound/rev/d9456378c12d (follow-up patch)
Whiteboard: [inbound]
Assignee | ||
Comment 17•13 years ago
|
||
backed out the push because of orange http://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=d9456378c12d
Whiteboard: [inbound]
Updated•13 years ago
|
Keywords: checkin-needed
Whiteboard: check in needed for "follow up patch"; see also bug 668351
Comment 18•13 years ago
|
||
Follow-up patch: http://hg.mozilla.org/integration/mozilla-inbound/rev/da3b0e3fadc7
Keywords: checkin-needed
Whiteboard: check in needed for "follow up patch"; see also bug 668351 → [follow up patch on inbound]
http://hg.mozilla.org/mozilla-central/rev/da3b0e3fadc7
Whiteboard: [follow up patch on inbound]
You need to log in
before you can comment on or make changes to this bug.
Description
•